Skip to content

[WIP] Refactor Dockerfile #8429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Raimo33
Copy link

@Raimo33 Raimo33 commented Jul 28, 2025

This is a draft PR to refactor the dockerfile according to issue #8409

changes:

  • remove useless python-builder stage
  • verify GPG keys (Docker: verify GPG keys #8405)
  • only include the necessary binaries in final image
  • use of ARG instead of ENV for build-time vars
  • remove useless apt packages
  • use of ADD instead of curl
  • set global SHELL
  • reduce redundancy of multi-arch image selections
  • use of tee + heredocs instead of messy echo's
  • stripped compiled binaries for smaller image
  • remove manual tini in favor of docker --init flag
  • avoid compiling sqlite, zlib, postgres from source
  • replace poetry with uv
  • avoid downloading all history/branches for submodules
  • parallelize make and submodules cloning
  • better docker caching of layers
  • fix CPU compatibility bug (Dockerfile is not CPU agnostic #8456)
  • avoid running cargo with QEMU. true cross compilation
  • avoid QEMU completely
  • replace messy cargo config.toml file edit with ENV vars
  • avoid installing clippy and rust-doc components
  • any host to any target image building (not tested)
  • avoid installing manpages

Improvements:

Feature Before After % Improvement
Build Time XX min XX min XX%
Image Size XX MB XX MB XX%
Lines of Code XXX XXX XX%

@ShahanaFarooqui
Copy link
Collaborator

@Raimo33 As per @cdecker's suggestion here, we should modify the Dockerfile to run as a non-root user for improved security. Since we're already refactoring the Dockerfile, could we incorporate this change as well?

@Raimo33
Copy link
Author

Raimo33 commented Aug 2, 2025

I'm removing the manual handling of tini in favor of a user controlled approach, more docker native and friendly.

ENTRYPOINT  [ "/usr/bin/tini", "-g", "--", "./entrypoint.sh" ]

becomes:

ENTRYPOINT  ["/entrypoint.sh"]

and the user is free to run the container with tini by using the official --init flag as follows:

docker run --init ...

@Raimo33 Raimo33 force-pushed the docker-refactor branch 3 times, most recently from 30c3c61 to db48b79 Compare August 6, 2025 11:06
@Raimo33
Copy link
Author

Raimo33 commented Aug 6, 2025

The Dockerfile now should also support building the image from any host architecture as well. It previously only suppported amd64 to any. now it should be any to any, but I've no way of testing it

@Raimo33 Raimo33 force-pushed the docker-refactor branch 3 times, most recently from 008f67c to ac0910c Compare August 10, 2025 13:19
@Raimo33
Copy link
Author

Raimo33 commented Aug 11, 2025

make install currently calls git describe to know the version of the program. This means that the .git/ folder needs to be copied into the Dockerfile for it to work. But copying the .git/ folder almost always invalidates the cache of the COPY . . command.

I'd like to explore ways to not include the .git/ folder (aka placing it in the .dockerignore). Maybe we can add an option to the Makefile that doesn't call git describe

@Raimo33 Raimo33 force-pushed the docker-refactor branch 5 times, most recently from e264a55 to c145582 Compare August 12, 2025 01:06
@cdecker
Copy link
Member

cdecker commented Aug 12, 2025

Nice changes, and with the switch to uv things may get even a bit simpler still. Undraft once ready, and we'll review again and merge asap 👍

@Raimo33
Copy link
Author

Raimo33 commented Aug 12, 2025

i dont want to undraft until these issues are addressed:

@Raimo33 Raimo33 force-pushed the docker-refactor branch 5 times, most recently from 71ca3c8 to 66a9497 Compare August 20, 2025 19:46
@Raimo33
Copy link
Author

Raimo33 commented Aug 21, 2025

When compiling for armv7 I'm getting this unaligned access error:

#49 7.927 checking for unaligned access to int... ccan/tools/configurator/configurator: Test for HAVE_UNALIGNED_ACCESS did not compile:
#49 7.959 configuratortest.c: In function 'main':
#49 7.959 configuratortest.c:8:22: error: array subscript 'int[0]' is partly outside array bounds of 'char[4]' [-Werror=array-bounds]
#49 7.959     8 |         return *x == *y;
#49 7.959       |                      ^~
#49 7.959 configuratortest.c:5:11: note: at offset 1 into object 'pad' of size 4
#49 7.959     5 |      char pad[sizeof(int *) * 1];
#49 7.959       |           ^~~
#49 7.959 cc1: all warnings being treated as errors
#49 7.960 
#49 ERROR: process "/bin/bash -euo pipefail -c ./configure --prefix=/tmp/lightning_install --enable-static --disable-compat --disable-valgrind" did not complete successfully: exit code: 3

This is because armv7 does not guarantee unaligned access support, so the compilation should rightfully fail. Now i wonder, are we sure that this repo is meant for armv7 as well? should we remove armv7 support completely? as far as I know it is only useful for raspberry pi 1 and 2. Raspberry PI's 3,4,5 have armv8 64bit.

I think someone should check if this compilation still works: (https://github.com/ElementsProject/lightning/blob/master/doc/getting-started/getting-started/installation.md#to-cross-compile-for-raspberry-pi)

@Raimo33
Copy link
Author

Raimo33 commented Aug 21, 2025

@cdecker Adding a non-root USER directly inside the Dockerfile is not a reliable option when using mounted volumes. The reason is that file permissions on mounted volumes are determined by the UID and GID of the host system, not the container. Hardcoding a UID, for example USER 1000:1000, might work in some environments, but it is not guaranteed to match the host system’s user. On many systems, the first regular user is UID 1000, but this is not universal, and in environments like CI/CD runners, containers, or systems with different user configurations, the UID/GID may differ.

Hardcoding the UID inside the Dockerfile creates a fragile assumption and can easily lead to permission issues. Some repos such as mempool do just that, but I don't deem it robust.

I have fought with docker user permissions for years, and when there are volumes involved, unless using docker-compose, the best thing is running as root. Other ways are hacky workarounds that involve setting UID and GID specifically. docker-compose can abstract away these hacks but it's not our case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Dockerfile after the removal of all built-in Python plugins
3 participants